feat(sandbox): Enforce network_mode allowlist on docker#785
Conversation
network_mode is now the authoritative network control across the launch path; the deprecated allow_internet boolean is a derived shim. Previously every backend keyed off allow_internet, so allowed_hosts never reached the sandbox and network_mode: allowlist was validated but unenforceable - rejected at preflight on every backend. - allowlist on the docker sandbox is now ENFORCED: the agent container joins an internal (no-egress) network and its HTTP(S) traffic routes through a stdlib egress-proxy sidecar that forwards only to allowed_hosts. Other hosts, raw-IP connections, and proxy-ignoring tools have no route off-box (default deny). New: sandbox/_egress.py, _egress_proxy.py. - sandbox/network_policy.py resolves the effective policy (open/block-all/ allowlist) and gates allowlist to backends that can enforce it; daytona/ modal fail closed (never silently open). - runtime_capabilities: allowlist supported on docker, rejected at preflight elsewhere with a clear message. - setup.py: preserve_agent_network lifts ANY restrictive policy (no-network OR allowlist) to public for web-disabled agent runs (they need the model API; no-web is enforced at the agent layer). - docs/task-authoring-task-md.md network-policy section + CHANGELOG. Verified on real docker (egress e2e): allowed host -> 200, non-allowed -> blocked, direct socket with proxy stripped -> no route. Full suite green.
Review (automated thorough pass) — ✅ no blocking defectsI ran an adversarial correctness/security review of the allowlist enforcement plus the targeted test + lint suite on a fresh checkout of the PR head. The enforcement is genuinely sound. Key claims verified:
Tests/lint: Non-blocking nits (follow-ups, not merge blockers)
Verdict: MERGE-READY (with the above as follow-ups). Leaving the actual merge to @Yiminnn since the branch was updated very recently and may still be iterating. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Wildcard allowed_hosts: a single leading `*.` label (e.g. *.example.com) matches subdomains at any depth (Harbor/nginx semantics) but not the apex; mid/trailing wildcards are rejected at parse time. Validator in task/config.py, matcher in sandbox/_egress_proxy.py. Model lane: a restrictive network_mode (no-network or allowlist) on docker now keeps one always-allow lane to the host-side benchflow model proxy open, so an agent run reaches the model without opening the sandbox to the public internet (no-network becomes model-only egress). The egress sidecar permits the docker host (_docker_host_address()) and routes to it via the bridge gateway / host-gateway. Replaces the blanket lift-to-public for web-disabled docker runs (extracted to _lift_agent_network_to_public, now docker-excluded); other sandboxes keep the lift. allow_model_endpoint:false (default true) closes the lane for a hermetic, no-model run. Live-verified through the real egress proxy: lane host + allowed host reachable, non-allowed host blocked (403); wildcard subdomain reachable, apex blocked. ENG-219
Greptile SummaryThis PR makes
Confidence Score: 4/5The core egress enforcement logic is sound and the lockdown verification loop prevents silent policy failures, but restore() in docker.py still reconnects restored containers to the unrestricted public bridge — an outstanding gap flagged in earlier review threads that this PR does not address. The new egress proxy, policy resolution, and lockdown_complete guard are all well-designed and tested. The two new inline comments are minor (IPv6 port parsing, custom-network error clarity) and do not affect the main enforcement path. The unresolved restore() bypass remains present and is the primary reason confidence falls short of the maximum. src/benchflow/sandbox/docker.py — specifically the restore() method which bypasses egress network enforcement after a snapshot restore. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant rollout as Rollout
participant docker as DockerSandbox
participant compose as docker compose
participant main as main container
participant sidecar as bf-egress sidecar
participant internet as Internet
rollout->>docker: "start() [_network_locked=False]"
docker->>compose: up --wait (no egress override)
compose->>main: start (on project_default, full internet)
rollout->>docker: relock_network(extra_allowed_hosts)
docker->>docker: "_network_locked = True"
docker->>docker: build_egress_override() write JSON
docker->>compose: up --no-deps bf-egress (egress override now in paths)
compose->>sidecar: start (on bf_egress_internal + bf_egress_external)
docker->>main: docker network connect bf_egress_internal
docker->>main: docker network disconnect project_default
docker->>main: docker inspect verify attached networks
main->>docker: lockdown_complete() check
alt lockdown verified
docker->>rollout: return HTTP_PROXY env
else lockdown failed
docker->>rollout: raise SandboxStartupError
end
rollout->>main: exec agent with proxy env
main->>sidecar: HTTP(S) via proxy bf-egress:8080
sidecar->>sidecar: _host_allowed(hostname)?
alt allowed host
sidecar->>internet: forward origin-form rewrite for HTTP
else denied
sidecar->>main: 403 Forbidden
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant rollout as Rollout
participant docker as DockerSandbox
participant compose as docker compose
participant main as main container
participant sidecar as bf-egress sidecar
participant internet as Internet
rollout->>docker: "start() [_network_locked=False]"
docker->>compose: up --wait (no egress override)
compose->>main: start (on project_default, full internet)
rollout->>docker: relock_network(extra_allowed_hosts)
docker->>docker: "_network_locked = True"
docker->>docker: build_egress_override() write JSON
docker->>compose: up --no-deps bf-egress (egress override now in paths)
compose->>sidecar: start (on bf_egress_internal + bf_egress_external)
docker->>main: docker network connect bf_egress_internal
docker->>main: docker network disconnect project_default
docker->>main: docker inspect verify attached networks
main->>docker: lockdown_complete() check
alt lockdown verified
docker->>rollout: return HTTP_PROXY env
else lockdown failed
docker->>rollout: raise SandboxStartupError
end
rollout->>main: exec agent with proxy env
main->>sidecar: HTTP(S) via proxy bf-egress:8080
sidecar->>sidecar: _host_allowed(hostname)?
alt allowed host
sidecar->>internet: forward origin-form rewrite for HTTP
else denied
sidecar->>main: 403 Forbidden
end
Reviews (14): Last reviewed commit: "fix(network): address multi-agent audit ..." | Re-trigger Greptile |
| # Plain HTTP: target is an absolute URI (http://host/path) in proxy form. | ||
| host = "" | ||
| if "://" in target: | ||
| host = target.split("://", 1)[1].split("/", 1)[0] | ||
| if not host: | ||
| for hl in header.split(b"\r\n"): | ||
| if hl.lower().startswith(b"host:"): | ||
| host = hl.split(b":", 1)[1].decode("latin-1").strip() | ||
| break | ||
| hostname = host.rsplit(":", 1)[0].strip("[]") | ||
| port = int(host.rsplit(":", 1)[1]) if ":" in host and "]" not in host else 80 | ||
| if not _host_allowed(hostname): | ||
| _deny(client, hostname) | ||
| return | ||
| try: | ||
| upstream = socket.create_connection((hostname, port), timeout=30) | ||
| except OSError: | ||
| client.sendall(b"HTTP/1.1 502 Bad Gateway\r\nConnection: close\r\n\r\n") | ||
| return | ||
| upstream.sendall(header) | ||
| _pipe(client, upstream) |
There was a problem hiding this comment.
Plain-HTTP request forwarded in absolute-URI form
For plain-HTTP requests the proxy forwards the raw header bytes — including the proxy request-line (GET http://host/path HTTP/1.1) — directly to the upstream server. RFC 7230 §5.3.2 says servers MUST accept the absolute-form, but a non-negligible number of back-ends (nginx with default config, some Python WSGI servers, and many embedded HTTP stacks) return 400 Bad Request for requests that arrive in absolute-URI form, which would cause tasks to see inexplicable failures for allowlisted plain-HTTP hosts.
The fix is to rewrite the request-line before forwarding: extract the path component from target (already split at line 122), reconstruct METHOD /path HTTP/1.1\r\n, and replace the first line of header before calling upstream.sendall.
| "NO_PROXY": "localhost,127.0.0.1", | ||
| "no_proxy": "localhost,127.0.0.1", | ||
| }, | ||
| "depends_on": [_EGRESS_SERVICE], |
There was a problem hiding this comment.
depends_on list form doesn't wait for proxy readiness
Using a plain list for depends_on tells Compose to start main as soon as bf-egress has been created, not once it is actually listening on port 8080. In practice the proxy binds almost instantly, but on a slow or loaded host the main container can fire its first proxied request (e.g. during agent framework import) before srv.accept() is running, resulting in Connection refused with no retry path. Switching to the long-form with condition: service_started is equivalent but explicitly documented; condition: service_healthy with a TCP healthcheck would eliminate the race entirely.
| "depends_on": [_EGRESS_SERVICE], | |
| "depends_on": { | |
| _EGRESS_SERVICE: {"condition": "service_started"}, | |
| }, |
There was a problem hiding this comment.
Fixed in 0c78eadd35647bc47fb7bb4536d64d8555696589.
The egress override now gives bf-egress a TCP healthcheck and makes main depend on service_healthy. relock_docker_network() also fails closed unless the sidecar container reports healthy before returning HTTP(S)_PROXY to the agent, so the proxy env is no longer exposed after mere service creation.
Validation on the pushed head:
uv run pytest tests/test_network_policy.py tests/test_network_modes.py tests/test_network_runtime.py -q-> 62 passeduv run ruff check src/benchflow/sandbox/_egress.py src/benchflow/sandbox/docker_network_lockdown.py tests/test_network_policy.py tests/test_network_runtime.pyuv run ruff format --check src/benchflow/sandbox/_egress.py src/benchflow/sandbox/docker_network_lockdown.py tests/test_network_policy.py tests/test_network_runtime.pyuv run ty check src/benchflow/sandbox/_egress.py src/benchflow/sandbox/docker_network_lockdown.py- GitHub CI is green on
0c78eadd3, includingtest,pip-audit,manifest-parity, andintegration-light / rollout-smoke. - The
integration-light-jobsartifact validates healthy withbenchflow-experiment-review: ACP trajectory, LLM trajectory,results.jsonl, reward present, 334350 tokens, 18 tool calls, andtraining_ready=1.
Moving the PR back to status:ready; keeping review:changes-requested because the security-sensitive PR still needs human re-review to clear the existing review decision.
| try: | ||
| upstream = socket.create_connection((hostname, port), timeout=30) | ||
| except OSError: | ||
| client.sendall(b"HTTP/1.1 502 Bad Gateway\r\nConnection: close\r\n\r\n") | ||
| return | ||
| upstream.sendall(header) | ||
| _pipe(client, upstream) | ||
| upstream.close() |
There was a problem hiding this comment.
upstream socket not closed on exception in the plain-HTTP path
upstream is created at line 134, but if upstream.sendall(header) (line 138) raises OSError the outer except Exception: pass silently swallows the error and upstream.close() (line 140) is never called. In CPython this is reclaimed by reference-counting when _handle returns, but in PyPy/non-reference-counted runtimes the descriptor leaks until GC. Wrapping the post-create_connection block in a try/finally (or using upstream as a context manager) would make cleanup explicit and deterministic.
New docs/network-mode-prior-art.md records the network-mode design model (no-network/public/allowlist + wildcard + model lane), a mode/mechanism taxonomy, and credits to the prior-art platforms the design draws on (Harbor, Modal, AISI Inspect, WAREX, SWE-bench, SWE-bench-Live, WebArena, Cybench, tau2-bench, browser-use) with primary-source links. Registered in the Mintlify nav and cross-linked from the task-authoring guide and sandbox-hardening. Citations verified against primary sources; corrects the Harbor PR#1854 framing (wildcard-depth clarification, not the feature origin) and notes the "N0/N1/N2" shorthand is ours, not AISI nomenclature. ENG-219
Network-mode verification —
|
strategy (environment.network_mode) |
docker | daytona |
|---|---|---|
| public | ✅ reward 1.0 | ✅ reward 1.0 |
allowlist — full (*.ncbi.nlm.nih.gov … + agent/model hosts) |
-32603 (model 404 — see note 3, not a proxy block) |
🚫 preflight REJECT (fail-closed) |
allowlist — deny (example.com only) |
🚫 ERR — proxy 403'd agent install | 🚫 preflight REJECT |
| no-network | 🚫 ERR — no route, agent can't install | ✅ reward 1.0 (see note 2) |
hermetic (no-network + allow_model_endpoint:false) |
🚫 ERR — no route | ✅ reward 1.0 (see note 2) |
docker batch — Score: 1/5, errors=4:
[ERR] citation-check-nonet (Agent openhands install failed (rc=127))
[ERR] citation-check-deny (Agent openhands install failed (rc=127))
[ERR] citation-check-hermetic (Agent openhands install failed (rc=127))
[ERR] citation-check-wildcard (ACP error -32603: Internal error)
Job complete: 1/5 (20.0%), errors=4, time=7.2min # the 1 pass = public (reward 1.0)
daytona batch — Score: 3/5, errors=2:
benchflow.task.runtime_capabilities.UnsupportedTaskFeatureError:
- environment.network_mode: network_mode='allowlist' is enforced only on the 'docker' sandbox
(egress proxy); not available on this sandbox — use 'docker', 'no-network', or 'public' (ENG-219) (sandbox=daytona)
[ERR] citation-check-deny (Task uses parsed runtime features that BenchFlow…)
[ERR] citation-check-wildcard (Task uses parsed runtime features that BenchFlow…)
Job complete: 3/5 (60.0%), errors=2, time=6.2min # passes = public, nonet, hermetic
3 · The proxy is enforcing in a live agent run (the load-bearing evidence)
For the deny variant the egress override is wired straight from the task.md, and the sidecar actively 403s every non-allowlisted host the agent reaches:
# applied egress override (from task.md allowed_hosts: [example.com])
{'ALLOWED_HOSTS': 'example.com', 'PORT': '8080', 'BENCHFLOW_EGRESS_LANE_HOST': '172.17.0.1'}
# openhands install log — proxy (172.21.0.2:8080) denies the Ubuntu mirrors (not allowlisted):
E: Failed to fetch http://archive.ubuntu.com/.../InRelease 403 Forbidden [IP: 172.21.0.2 8080]
E: Failed to fetch http://security.ubuntu.com/.../InRelease 403 Forbidden [IP: 172.21.0.2 8080]
When those install hosts are allowlisted, the proxy forwards them and the agent installs cleanly (apt over plain-HTTP through the proxy worked — no absolute-URI 400). So the allowlist permits exactly the listed hosts and denies the rest, in a real run.
Notes / findings
-
Model lane covers
host.docker.internal, but this deployment calls the model provider directly. The agent env setsLLM_BASE_URL=https://api.deepseek.com(not a host-side LiteLLM proxy athost.docker.internal:<port>), so the lane (172.17.0.1) is inert here and the real provider host must be allowlisted for a restrictive run. The lane mechanism itself is sound (verified separately: a host listener at172.17.0.1is reachable through the sidecar), but its automatic benefit only fires in the host-proxy topology. → worth folding into ENG-262 (the lane should also admit the resolved provider host under a restrictive policy, since that host is known at launch). -
docker vs daytona no-network strictness differs. docker enforces
no-networkfrom the start — the agent can't even install (rc=127). daytona'sno-networklet the agent install, reach the model, and solve (nonet/hermetic→ reward 1.0). daytona backends still readallow_internetdirectly rather than routing throughresolve_network_decision(the ENG-262 C1 defense-in-depth item), and appear to install the agent before applying the policy. Flagging as a real cross-backend inconsistency. -
The
-32603on dockerallowlistis a model-routing quirk, not a network block.deepseek-v4-flash404s on the configuredapi.deepseek.com(the model is actually served by a separate internal endpoint); even the public run hit 37×404before succeeding via retries to the working endpoint. The proxy permitted every listed host (install succeeded through it); the agent just couldn't complete the model call cleanly under the tighter time budget. Orthogonal to this PR.
Bottom line: the network_mode enforcement is solid — policy resolution is correct on both sandboxes, docker enforces allowlist/no-network (live 403s on unlisted hosts, agent install blocked under restrictive policy), daytona fails closed on allowlist at preflight, and allow_model_endpoint:false correctly closes the lane. The two items above (model-lane direct-provider coverage, daytona no-network strictness) are follow-ups, both already in ENG-262's scope.
Derive daytona block-all from resolve_network_decision (not the deprecated allow_internet bool) and verify it after start with a raw-TCP egress canary; abort with SandboxStartupError if a block-all policy can still reach the network (platform did not honor network_block_all).
…ctive policy (ENG-263) Under no-network/allowlist the agent cannot silently fall back to the direct provider (the egress allowlist blocks it) — so a skipped/unavailable LiteLLM usage proxy must abort the run instead of leaving the agent pointed at a blocked, untracked provider endpoint. Adds network_is_restrictive + proxy_unavailable_is_fatal and threads it through ensure_litellm_runtime.
…263) The container now comes up open so the agent can install; relock_network() then drops it off the public bridge (and, for allowlist/model-lane, starts the bf-egress sidecar + moves it onto the internal-only net, returning HTTP_PROXY for the agent). The main container is never recreated, so the install survives. Called from install_agent() after lockdown_paths. Fixes rc=127 agent-install failures under no-network/tight-allowlist on docker.
…s probe) (ENG-263) The daytona no-network leak was the _lift_agent_network_to_public blanket lift to public (the same P0 the model lane replaced for docker), still firing on non-docker. Gate the lift for daytona/daytona-dind too (no lane there -> the honest behavior is to keep the policy and fail closed). Also fix the egress canary probe to a single-line python -c (the heredoc form did not execute through daytona exec) so block-all enforcement is actually verified at start. Verified live: daytona no-network now resolves block_all=True and the canary (1.1.1.1:443) is unreachable -- genuinely offline, no longer silently public.
|
Automation triage (2026-06-16): still belongs in |
Resolves conflicts from #787 (Mintlify docs retirement): - docs/docs.json: honored main's deletion (#787 retired the Mintlify config); the network-mode-prior-art nav entry is moot. - docs/sandbox-hardening.md: kept main's reworded task-authoring.md line and Yimin's network-mode-prior-art cross-link. Code merged cleanly. Verified: test_network_modes + test_network_policy + test_internet_policy + test_runtime_capabilities = 92 passed; ruff + format + ty clean on the sandbox surface.
|
Automation (2026-06-16): merged The only conflicts were docs from #787 (Mintlify config retirement):
Code merged cleanly (config.py / test files auto-merged, no logic conflicts). Re-verified locally: PR should now be mergeable and CI can run on the fresh head. Still needs a human review (security-sensitive egress enforcement) + the docker/daytona network-mode E2E re-run before merge — removing |
Runtime-behavior follow-up (ENG-263) — fixes + live verification on both sandboxesVerifying this PR's enforcement with a Fix #1 — docker install-before-lockdown (was: agent install
|
|
Users Simulation automation follow-up (2026-07-06): GitHub CI is now green on head |
|
Users Simulation automation review (2026-07-08): simulation ready, waiting on human re-review on head Evidence:
Caveat: the bundled review-skill fixture artifacts Metadata caveat: GLM artifact token/timing coverage is healthy, but Labels: |
|
Daily scan follow-up (2026-07-09): restoring The original human decomposition/security blockers appear addressed on current head Please add a small readiness guard for the egress sidecar, preferably a healthcheck plus wait/poll before returning proxy env from relock, then rerun the focused network/runtime tests and rollout smoke. After that this can move back to |
|
Users Simulation automation review (2026-07-09): blocked on current head What is healthy now:
Remaining blocker:
Please add a small readiness guard for |
|
Daily scan follow-up (2026-07-10): re-reviewed the new head What changed since the last blocked comment:
Validation on the PR head in
Moved labels from |
|
Users Simulation automation review (2026-07-10): blocked on current head What passed:
Blocking issue:
Please either reject that configuration up front, or keep the runtime on a proxy path that is compatible with the declared policy so Additional hardening note: Docker restore is now blocked under restrictive policies, but the Daytona snapshot restore path still recreates from snapshot without an explicit re-lock step. I did not prove a bypass in the current suite, but that is the remaining restore path I would harden before calling this ready. Moving back to |
|
Users Simulation automation follow-up (2026-07-10): ready by simulation on current head The fresh restrictive-model-lane blocker from #785 (comment) is addressed by
Validation:
Moving the PR back to |
|
Daily scan hygiene (2026-07-11): latest automation evidence says the network-mode changes are functionally ready by simulation, but GitHub still reports this PR as Label update: moving |
|
Users Simulation automation review (2026-07-11): functionally ready by simulation, still blocked by merge/review gates on head Scope checked: Evidence:
Artifact health:
Thermo review: functionally acceptable but still security-sensitive. The new network-policy helpers are large but cohesive and tested; no immediate fail-open blocker remains in this simulation pass. Merge gate remains blocked because GitHub reports |
|
Users Simulation automation follow-up (2026-07-11): ready by simulation / ready for human re-review on current head What changed:
Validation:
Current blocker state:
|
Problem
network_modeis the documented outbound-network control, but the launch pathhistorically keyed off the deprecated
allow_internetboolean. That madeallowlistvalidated but unenforceable: a task could declareallowed_hosts,yet the sandbox would either run open or reject too late.
Current fix
This PR makes
network_modeauthoritative and fail-closed across the runtimepath.
maincontainer is moved onto aninternal no-egress network after agent install, and HTTP(S) traffic goes
through a stdlib egress proxy sidecar that forwards only to
allowed_hostsplus the model lane when allowed.
Daytona's IPv4 CIDR control when faithfully expressible; hosts are pinned in
/etc/hosts; wildcard, unresolvable, over-limit, and DinD cases fail closed.allow_model_endpoint: falseprevents the provider/modelhost from being appended to Docker/Daytona allowlists and now fails closed
before model-backed agent launch.
restrictive network policies instead of reconnecting restored containers to
the public compose bridge.
bf-egresssidecar beforereturning proxy env.
docker_network_lockdown.pyanddaytona_network_lockdown.py;docker.pyand
daytona.pyremain below the 1k-line review threshold.Latest validation
Current head:
c561ab7881f8f6418826684d5a50a6f5cbab7edd.origin/main; resolved conflicts only inCHANGELOG.mdandtests/test_internet_policy.py.uv sync --extra dev --locked-> passeduv run pytest tests/test_network_modes.py tests/test_network_policy.py tests/test_network_runtime.py tests/test_connect_as_env.py tests/test_internet_policy.py tests/test_runtime_capabilities.py -q-> 120 passeduv run ruff check .-> passeduv run ruff format --check src tests tools-> passeduv run ty check src/-> passeduv run bench --help-> passedc561ab788:test,pip-audit,manifest-parity / parity,integration-light / rollout-smoke, and detect-scope all passed.integration-light-jobsartifact validated healthy withbenchflow-experiment-review: 1/1 rollout healthy; ACP trajectory, LLMtrajectory, and training-ready
results.jsonlpresent; 234,509 tokens, 14tool calls, reward present.
Remaining gate
The branch is no longer dirty and GitHub reports it as mergeable, but the stale
CHANGES_REQUESTEDreview decision still blocks merge. This is ready forsecurity/human re-review; no known code/config blocker remains.